fix(sagemaker): improve SSH configuration error handling for SageMakare remote connections #8328
Conversation
…r remote connections
|
bhavya2109sharma
force-pushed
the
fix/ssh-255-error
branch
3 times, most recently
from
618470b to
e2e9039
Compare
| 'This space requires remote access to be enabled.\nWould you like to restart the space and connect?\nAny unsaved work will be lost.' | ||
|
|
||
| export const SshConfigErrorMessage = () => { | ||
| return `Unable to connect. ~/.ssh/config contains invalid SSH configuration. Fix the errors to continue.` |
There was a problem hiding this comment.
Do we write the "~" character in other error messages? Windows doesn't use it so we shouldn't explicitly write this. We can possibly show it if we have confirmed it is applicable to the user's environment. Alternatively, we display the exact path, though it may be a bit too long.
There was a problem hiding this comment.
Here existing pattern in the codebase does use ~/.ssh/config
| logger.error(`sagemaker: failed to add ssh config section: ${err.message}`) | ||
|
|
||
| if (err instanceof ToolkitError && err.code === 'SshCheckFailed') { | ||
| const sshConfigPath = path.join(os.homedir(), '.ssh', 'config') |
There was a problem hiding this comment.
Can we use the getSshConfigPath() helper method?
| // Format stderr error message | ||
| let errorMessage = result.stderr?.trim() || `ssh check against host failed: ${result.exitCode}` | ||
| const sshConfigPath = getSshConfigPath() | ||
| errorMessage = errorMessage.replace(new RegExp(`${sshConfigPath}:\\s*`, 'g'), '').trim() |
There was a problem hiding this comment.
This could benefit from a comment explaining the purpose.
| @@ -0,0 +1,4 @@ | |||
| { | |||
| "type": "Bug Fix", | |||
| "description": "SageMaker: Enhanced SSH configuration error handling to show user-friendly modal dialogs with line numbers and an \"Open SSH Config\" button for quick fixes." | |||
There was a problem hiding this comment.
Here are some suggestions regarding change log entries:
- Be objective, avoid wording like "user-friendly" and "quick"
- Do not end in a period as they are list items (which generally don't use punctuation)
- Avoid the word "Enhanced", prefer "Improved" instead (appears 10+ times in existing change log)
- Use AI to help rephrase in the existing style (have it read the CHANGELOG.md file)
Here's an alternative wording (AI-assisted):
SageMaker: SSH configuration errors now display line numbers and include an "Open SSH Config" button
| /** | ||
| * Helper function to determine if an error should be suppressed (already shown to user) | ||
| */ | ||
| function shouldSuppressError(err: any): boolean { |
There was a problem hiding this comment.
When I read this method name, I generate many questions in my head.
- Why are we suppressing errors?
- In which context is it appropriate to suppress errors?
- When is it appropriate to update this method? What all is referencing this method?
- How does this method know which errors should be suppressed?
- How do we know this error was already shown to the user? (A question I would have anyway)
Part of the problem is that this method is making a decision (made clear by the word should), which realistically can't be made without context that isn't provided to the method. An example of such context is the comment which says "Suppress errors that were already shown to user". That is something this method cannot possibly know.
To improve, we can simply rename the method (e.g., isUserConfigurationError, isUserCancelledError), or use error metadata (isUserCancelledError() is an existing helper method in the repository for this use case).
bhavya2109sharma
force-pushed
the
fix/ssh-255-error
branch
from
e2e9039 to
5b6ea86
Compare
…r remote connections
bhavya2109sharma
force-pushed
the
fix/ssh-255-error
branch
from
d67fec7 to
768b58a
Compare
2 participants
Problem
When attempting to connect to a SageMaker Space, if the user's SSH config has syntax errors (even outside the sm_* section), users would see a cryptic error message:
ssh check against host failed: 255This error message:
Didn't explain what was wrong
Didn't indicate it was an SSH config issue
Didn't provide a way to fix the problem
Solution
Enhanced the SSH configuration error handling to provide a better user experience:
Shows a clear message explaining the issue.
Displays the actual SSH error (including line numbers)
Provides an "Open SSH Config" button to directly open the config file for editing
Screen.Recording.2025-11-19.at.3.43.38.PM.mov
feature/xbranches will not be squash-merged at release time.